fix(hubspot): simplify integration to only create contacts#7147
fix(hubspot): simplify integration to only create contacts#7147
Conversation
The HubSpot lead tracking integration was creating and updating companies from Flagsmith org data, overwriting enriched company names with user-entered org names. HubSpot already handles company creation and contact association automatically from email domains. Simplify create_lead to only create the contact. Remove domain filtering from should_track since HubSpot handles that natively. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
Docker builds report
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7147 +/- ##
==========================================
+ Coverage 98.34% 98.36% +0.02%
==========================================
Files 1336 1348 +12
Lines 50161 50570 +409
==========================================
+ Hits 49331 49745 +414
+ Misses 830 825 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
|
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
So I get the overall intent of this PR. Although there is a side-effect that feels critical to me -> We are breaking the synchronization between flagsmith and hubspot (even in read-only) and we might be unable to update the subscription to Hubspot.
My understanding with this code is that hubspot will be completely in control of creating the organisation AND the association of the contacts. That makes sense.
Right now, _get_organisation_hubspot_id is in charge of finding the organisation and creating the HubspotOrganisation record. It became dead code and we are never storing the information on our side.
It becomes a problem in update_company_active_subscription that is now a no-op (we don't have hubspot_id) and I feel using the domain or the name is too fragile.
My proposal
The fix should keep _get_organisation_hubspot_id in create_lead and drop the name= overwrite and keep writing flagsmith_organisation_id to HubSpot.
Optionally we can remove associate_contact_to_company to make our intent 100% clear and remove the associated tests (that just confirms it's never called right?).
Let me know what you think and i'll re-review with your thinking in mind.
If aligned, here is a final assist 😄
For Claude
1. In _get_organisation_hubspot_id, remove the name=organisation.name parameter from the self.client.update_company(...) call but keep hubspot_company_id and flagsmith_organisation_id.
2. In create_lead, keep the call to _get_organisation_hubspot_id but remove associate_contact_to_company and the early returns tied to it.
3. Update/remove tests that assert associate_contact_to_company behaviour.
4. Remove any now-unused imports (e.g. re2/re if domain filtering stays removed).
The Flagsmith integration writing subscription/plan data to HubSpot was redundant (ChargeBee sync handles this daily) and caused confusion - trial subscriptions were being treated as paid accounts, overwriting correct data. - Remove update_company_active_subscription method and task - Remove AFTER_SAVE hook on Subscription.plan - Remove _get_organisation_hubspot_id (wrote org name + plan to HubSpot) - Remove _get_hubspot_company_by_domain (only used by above) - Remove associated tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Holmus
changed the title
for more information, see https://pre-commit.ci
|
Thanks for the review @Zaimwa9 - good catch on update_company_active_subscription becoming a no-op. After thinking through it more, I went further than your proposal: I removed update_company_active_subscription entirely rather than fixing it. Why: We have a ChargeBee -> HubSpot sync that runs daily and writes subscription data (plan, MRR, billing status, contract dates) to HubSpot companies. It's the billing source of truth. Having the Flagsmith integration also write subscription data was: Redundant - ChargeBee sync already handles this The new commit removes update_company_active_subscription, its task, the AFTER_SAVE hook, and associated dead code (_get_organisation_hubspot_id, _get_hubspot_company_by_domain). |
Visual Regression16 screenshots compared. See report for details. |
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
Sorry to be annoying. Couple of NITs that we should take care of now:
- Code removal
- Unused variables
Then i'll be happy to approve assuming all ✅ on the functionality
|
|
||
| domain = user.email_domain | ||
|
|
||
| if settings.HUBSPOT_IGNORE_DOMAINS_REGEX and re.match( |
There was a problem hiding this comment.
Let's clean those HUBSPOT_XXX from the settings please
| contact_id=HUBSPOT_USER_ID, | ||
| company_id=HUBSPOT_COMPANY_ID, | ||
| ) | ||
| mock_client_existing_contact.associate_contact_to_company.assert_not_called() |
There was a problem hiding this comment.
This test name should reflect that associations are not created.
(Atm: test_create_lead__existing_hubspot_org__creates_contact_and_associates)
There was a problem hiding this comment.
Actually, associate_contact_to_company is only used in test now so we might want to clean it all up
2 participants
The HubSpot lead tracking integration was doing three things:
Problems with (2): Flagsmith org names like "ew", "Test", "Manejo-Org" overwrote enriched company names in HubSpot.
Problems with (3): This was redundant with our ChargeBee -> HubSpot sync (which runs daily and is the billing source of truth). Worse, it caused data confusion - trial subscriptions written by the integration were treated as paid accounts, triggering workflows that incorrectly set company type to Customer.
Changes
create_leadto only create the contact - HubSpot handles company creation and association from email domainsshould_trackto just check if the feature is enabledupdate_company_active_subscriptionand its AFTER_SAVE hook - ChargeBee sync is the source of truth for subscription data_get_organisation_hubspot_idand_get_hubspot_company_by_domain(dead code after above)What still works
HubspotOrganisationmodel is unchanged (existing records preserved)Review effort: 1/5 (almost entirely deletions)